Skip to content

feat: Update IDL#1050

Merged
Shaddoll merged 2 commits intomasterfrom
tl
Apr 29, 2026
Merged

feat: Update IDL#1050
Shaddoll merged 2 commits intomasterfrom
tl

Conversation

@Shaddoll
Copy link
Copy Markdown
Member

@Shaddoll Shaddoll commented Apr 27, 2026

What changed?

  • pull latest IDL
  • update corresponding mappers
  • suppress deprecation warnings in generated ActiveClusterSelectionPolicy.java file

Why?

  • Pull idl
  • The generated file creates a deprecation warning failing the build

How did you test it?
./gradlew build

Potential risks

Release notes

Documentation Changes

Comment thread build.gradle Outdated
Comment thread src/gen/java/com/uber/cadence/TaskListKind.java
@Shaddoll Shaddoll force-pushed the tl branch 2 times, most recently from a898f0b to 185dae5 Compare April 27, 2026 23:59
Comment thread build.gradle
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 28, 2026

Code Review 👍 Approved with suggestions 5 resolved / 6 findings

Updates the IDL while addressing multiple null-handling issues and deprecation warning configuration. Ensure replaceGeneratedSources is updated to handle missing files and improve idempotency.

💡 Edge Case: replaceGeneratedSources silently succeeds when file missing

📄 build.gradle:189-191 📄 build.gradle:184-198

When the generated file doesn't exist (e.g., protobuf generation failed or path changed), the task prints a message but returns successfully. Since compileJava depends on this task, the build will proceed and then fail later with a confusing deprecation/Werror compile error rather than a clear root cause.

Consider throwing a GradleException instead of just printing and returning, so the build fails fast with a clear message.

Suggested fix
if (!file.exists()) {
    throw new GradleException("File not found: $file")
}
✅ 5 resolved
Bug: taskList baseName mapping will NPE on null

📄 src/main/java/com/uber/cadence/internal/compatibility/proto/mappers/TypeMapper.java:159
The new taskList mapping calls .setBaseName(t.getBaseName()) directly. Since baseName is an optional field on the Thrift TaskList, getBaseName() can return null. Protobuf builders throw NullPointerException on null string values. Other nullable string fields in this file (e.g., activeClusterInfo at line 1033, clusterAttribute at line 1086) correctly use Helpers.nullToEmpty() to handle this.

Bug: New EPHEMERAL TaskListKind not mapped in EnumMapper

📄 src/gen/java/com/uber/cadence/TaskListKind.java:6 📄 src/main/java/com/uber/cadence/internal/compatibility/proto/mappers/EnumMapper.java:52-63 📄 src/main/java/com/uber/cadence/internal/compatibility/proto/mappers/EnumMapper.java:284-294
The IDL update adds EPHEMERAL to TaskListKind enum (TaskListKind.java:6), but EnumMapper.taskListKind() only handles NORMAL and STICKY in both directions. If a task list with EPHEMERAL kind is encountered at runtime, both the thrift→proto mapper (line 52-63) and proto→thrift mapper (line 284-294) will throw IllegalArgumentException. Since this IDL update is specifically adding this new kind, it should be mapped.

Bug: taskList.setName() lacks nullToEmpty unlike new baseName field

📄 src/main/java/com/uber/cadence/internal/compatibility/proto/mappers/TypeMapper.java:156 📄 src/main/java/com/uber/cadence/internal/compatibility/proto/mappers/TypeMapper.java:158
To address reviewer feedback on the previous finding: the Name field at line 156 has the same potential NPE as the previously-flagged baseName — proto's setName(null) throws NullPointerException. The new baseName code correctly uses Helpers.nullToEmpty() at line 158, but the pre-existing setName(t.getName()) at line 156 does not. While Name being null on a TaskList is unlikely in practice, the inconsistency suggests wrapping it too for safety. Note: the Name line is pre-existing code, so this is a low-priority suggestion.

Quality: Deprecation warnings silently disabled project-wide

📄 build.gradle:187 📄 build.gradle:200
The change from -Xlint:deprecation to -Xlint:-deprecation disables all deprecation warnings for both main and test compilation. With -Werror still active, this means deprecated API usage will no longer cause build failures. While this may have been needed to get the build passing with the new IDL, it suppresses all deprecation warnings globally, not just for the new code. Consider using a more targeted suppression (e.g., @SuppressWarnings("deprecation") on specific files) to retain deprecation checking for the rest of the codebase.

Bug: ActiveClusterSelectionPolicy mapper only maps 1 of 5 fields

📄 src/main/java/com/uber/cadence/internal/compatibility/proto/mappers/TypeMapper.java:1047-1061 📄 src/gen/java/com/uber/cadence/ActiveClusterSelectionPolicy.java:10-14
The activeClusterSelectionPolicy mapper in TypeMapper only maps the clusterAttribute field, leaving strategy, stickyRegion, externalEntityType, and externalEntityKey unmapped. While the tests explicitly track these as missing fields, this means any ActiveClusterSelectionPolicy with these fields set will silently lose data during thrift↔proto conversion. Since this PR is adding the mapper for the first time, it would be better to map all available fields now rather than accumulating tech debt.

🤖 Prompt for agents
Code Review: Updates the IDL while addressing multiple null-handling issues and deprecation warning configuration. Ensure replaceGeneratedSources is updated to handle missing files and improve idempotency.

1. 💡 Edge Case: replaceGeneratedSources silently succeeds when file missing
   Files: build.gradle:189-191, build.gradle:184-198

   When the generated file doesn't exist (e.g., protobuf generation failed or path changed), the task prints a message but returns successfully. Since `compileJava` depends on this task, the build will proceed and then fail later with a confusing deprecation/Werror compile error rather than a clear root cause.
   
   Consider throwing a `GradleException` instead of just printing and returning, so the build fails fast with a clear message.

   Suggested fix:
   if (!file.exists()) {
       throw new GradleException("File not found: $file")
   }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@Shaddoll Shaddoll merged commit d67cf58 into master Apr 29, 2026
13 of 15 checks passed
@Shaddoll Shaddoll deleted the tl branch April 29, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants